Add explicit overload to `Field` subclasses #1900

Viicos · 2024-01-04T20:46:43Z

In #1264 (comment), and to try working on bringing django-stubs and django-types together, I suggested a workaround that would allow type checkers to natively infer _ST and _GT without the need for a plugin.

Unfortunately, for it to work correctly it would most probably require higher kinded type vars (my current example doesn't play well with subclasses, and I can't use Self as it can't be parametrized).

This is an experimental test, and as you can see it adds a lot of verbosity. But this removes the need to a plugin to parametrize the type variables, and it support nullable fields.

Would you accept such a change? I'm ok if this isn't accepted, considering how much it adds

sobolevn

I like the idea. Several comments:

Please, delete the plugin code, so we can be sure that your changes are enough to pass our CI
Looks like stubstest is not happy with some of the changes. Please, fix or ignore them :)

Viicos · 2024-01-05T09:23:18Z

Great to hear! I'm also experimenting on another solution, but facing some issues (see microsoft/pyright#6914).

If this end up working, I think it might be preferable as it would avoid redefining all overloads, + people could easily subclass field classes.

Edit:

This alternative solution will require some fixes on both pyright and mypy, but it looks promising. On the pyright side, it is currently ~~being investigated~~ implemented. On the mypy side, it would require python/mypy#3737, and probably python/mypy#11366 (a fix that you created 😄). So I think it's best to wait a bit for this one.

Viicos · 2024-01-31T19:10:20Z

After looking at the related plugin code, I don't know if removing _pyi_private_set/get_type support is the best way. The thing is the following function:

django-stubs/mypy_django_plugin/transformers/fields.py

Lines 133 to 153 in 1735d9e

    
           def set_descriptor_types_for_field( 
        
               ctx: FunctionContext, *, is_set_nullable: bool = False, is_get_nullable: bool = False 
        
           ) -> Instance: 
        
               default_return_type = cast(Instance, ctx.default_return_type) 
        
               is_nullable = False 
        
               null_expr = helpers.get_call_argument_by_name(ctx, "null") 
        
               if null_expr is not None: 
        
                   is_nullable = parse_bool(null_expr) or False 
        
               # Allow setting field value to `None` when a field is primary key and has a default that can produce a value 
        
               default_expr = helpers.get_call_argument_by_name(ctx, "default") 
        
               primary_key_expr = helpers.get_call_argument_by_name(ctx, "primary_key") 
        
               if default_expr is not None and primary_key_expr is not None: 
        
                   is_set_nullable = parse_bool(primary_key_expr) or False 
        
               set_type, get_type = get_field_descriptor_types( 
        
                   default_return_type.type, 
        
                   is_set_nullable=is_set_nullable or is_nullable, 
        
                   is_get_nullable=is_get_nullable or is_nullable, 
        
               ) 
        
               return helpers.reparametrize_instance(default_return_type, [set_type, get_type])

Is being called before mypy actually infers the return type of a call to FooField, with the overloads provided in this PR. So I can't really take the "native" mypy solving logic into account. Besides, we can see primary key fields are special cased, so removing the plugin related logic here isn't trivial.

I also saw a couple hits of public code making use of these attributes on field subclasses.

The idea here would be to add tests without the plugin added in the mypy config, and assert that they work. I've tried making use of the parametrized feature of the pytest plugin, but looking at the CI it doesn't seem to play well.

I might take a second look and see if it's in fact possible to remove the plugin code

Viicos · 2024-02-01T21:36:50Z

django-stubs/contrib/auth/models.pyi

-    name = models.CharField(max_length=255)
-    content_type = models.ForeignKey(ContentType, on_delete=models.CASCADE)
-    codename = models.CharField(max_length=100)


Having explicit assignments feels a bit weird in a stub file and was causing issues when testing without the plugin (had error code var-annotated)

Viicos · 2024-02-01T21:37:57Z

django-stubs/contrib/auth/models.pyi

@@ -34,7 +35,7 @@ class GroupManager(models.Manager[Group]):
 class Group(models.Model):
    objects: ClassVar[GroupManager]

-    name = models.CharField(max_length=150)
+    name: models.CharField[str | int | Combinable, str]
    permissions = models.ManyToManyField(Permission)


... however I can't explicitly annotate permissions: models.ManyToManyField[..., ...] as the second type var is solved by the plugin to a class that doesn't exist in stubs nor at runtime

You can have a look here how it can be done: https://github.com/typeddjango/django-stubs/pull/2214/files#diff-0236ca9e909bc4743fa03e6653fc33adbaaef02a34d8698501a63a59ad624f1fR42-R51

Viicos · 2024-02-01T21:38:24Z

tests/typecheck/fields/test_base.yml

+                    mypy_django_plugin.main
+        -   mypy_section: |
+                [mypy]
+                disable_error_code = var-annotated


...So I had to disable the error code anyway when testing without the plugin

This allows type checkers other than mypy to infer correctly the `__set/get__` type variables, without the need for the plugin. The plugin logic to solve these type variables is kept, as some special casing is implemented to handle primary key fields with a default. The `Field` docstring was updated with an example reflecting this

This is required to run mypy without the plugin, as it would complain about these fields requiring an annotation. Using `follow_imports = silent` did not work in this case Also feels more natural than doing an assignment in a stub file

A bit of a hacky solution for now, maybe CI could be improved to add this kind of tests more easily. `var-annotated` is still an ignored error code as I wasn't able to change the assignment to an annotation for `ManyToManyField`s in my previous commit

Viicos · 2024-02-02T08:25:01Z

I rebased everything to have commits easier to review. CI is now passsing, except stubtests, as I had to make arguments following null as kw-only. This could be added to the allowlist but doesn't seem like a great solution. Any ideas how this could be handled?

I created a pull request @ mypy to add a --ignore-positional-only argument, might be more robust

Really not ideal, so open to alternatives

intgr · 2024-03-25T10:39:28Z

What's the status here, is it waiting for review?

@sobolevn since you commented, do you want to review this?

Viicos · 2024-03-25T10:59:40Z

What's the status here, is it waiting for review?

I think #1900 (comment) needs to be resolved in some way. Even if not really ideal, the linked mypy open PR seems to be the best option.

A slightly more elegant solution to this PR is described here, but unsupported in mypy currently.

This PR is also imo not a blocker for the next release

`db_default` is added on all overloads. `_ST` is replaced by the same set type set on `self`

Viicos · 2024-05-22T17:08:52Z

I think the additions from #2048 is what's breaking the when_default_for_primary_key_is_specified_allow_none_to_be_set test case.

flaeppe · 2024-07-13T22:11:41Z

I was playing around with getting rid of _ST and _GT in fields from related.pyi (ForeignKey and OneToOneField) and got stuck on python/mypy#14764 in regards to overloading on the null argument.

Now I'm thinking that python/mypy#14764 must be a problem for these changes too?

sobolevn reviewed Jan 5, 2024

View reviewed changes

Viicos commented Feb 1, 2024

View reviewed changes

Viicos force-pushed the overload-fields branch from 46928ae to c48f9c7 Compare February 2, 2024 07:44

Viicos added 3 commits February 2, 2024 08:46

Explicitly annotate some fields in the auth module

e31a5c6

This is required to run mypy without the plugin, as it would complain about these fields requiring an annotation. Using `follow_imports = silent` did not work in this case Also feels more natural than doing an assignment in a stub file

Viicos force-pushed the overload-fields branch from c48f9c7 to a075b0b Compare February 2, 2024 08:00

Add fields __init__ to allowlist

3dda6c8

Really not ideal, so open to alternatives

Viicos changed the title ~~Experimental: add explicit overload to Field subclasses~~ Add explicit overload to Field subclasses Feb 2, 2024

Viicos mentioned this pull request Feb 2, 2024

stubtest: Add a --ignore-keyword-only argument python/mypy#16861

Open

flaeppe mentioned this pull request Apr 10, 2024

Use field generic types for descriptors #2048

Merged

Incorporate latest changes

3a78871

`db_default` is added on all overloads. `_ST` is replaced by the same set type set on `self`

Viicos force-pushed the overload-fields branch from a32d5c8 to 3a78871 Compare May 22, 2024 16:41

Fix newly added tests

ec4f339

Viicos and others added 2 commits May 23, 2024 10:22

wip: Do not infer types from _pyi_private_*_type

b1e8662

[pre-commit.ci] auto fixes from pre-commit.com hooks

9512835

Viicos mentioned this pull request Jun 10, 2024

Add type hints to builtin models' fields #2214

Open

Viicos mentioned this pull request Aug 12, 2024

DecimalField inferred as Any Viicos/django-autotyping#71

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add explicit overload to `Field` subclasses #1900

Add explicit overload to `Field` subclasses #1900

Viicos commented Jan 4, 2024 •

edited

Loading

sobolevn left a comment

Viicos commented Jan 5, 2024 •

edited

Loading

Viicos commented Jan 31, 2024 •

edited

Loading

Viicos Feb 1, 2024

Viicos Feb 1, 2024 •

edited

Loading

flaeppe Jun 22, 2024

Viicos Feb 1, 2024

Viicos commented Feb 2, 2024 •

edited

Loading

intgr commented Mar 25, 2024

Viicos commented Mar 25, 2024 •

edited

Loading

Viicos commented May 22, 2024

flaeppe commented Jul 13, 2024

Add explicit overload to Field subclasses #1900

Are you sure you want to change the base?

Add explicit overload to Field subclasses #1900

Conversation

Viicos commented Jan 4, 2024 • edited Loading

sobolevn left a comment

Choose a reason for hiding this comment

Viicos commented Jan 5, 2024 • edited Loading

Viicos commented Jan 31, 2024 • edited Loading

Viicos Feb 1, 2024

Choose a reason for hiding this comment

Viicos Feb 1, 2024 • edited Loading

Choose a reason for hiding this comment

flaeppe Jun 22, 2024

Choose a reason for hiding this comment

Viicos Feb 1, 2024

Choose a reason for hiding this comment

Viicos commented Feb 2, 2024 • edited Loading

intgr commented Mar 25, 2024

Viicos commented Mar 25, 2024 • edited Loading

Viicos commented May 22, 2024

flaeppe commented Jul 13, 2024

Add explicit overload to `Field` subclasses #1900

Add explicit overload to `Field` subclasses #1900

Viicos commented Jan 4, 2024 •

edited

Loading

Viicos commented Jan 5, 2024 •

edited

Loading

Viicos commented Jan 31, 2024 •

edited

Loading

Viicos Feb 1, 2024 •

edited

Loading

Viicos commented Feb 2, 2024 •

edited

Loading

Viicos commented Mar 25, 2024 •

edited

Loading